The Static Analysis tool Coverity added that below code from functions js::RegExpMatcherRaw() and js::RegExpTesterRaw() don't affect the outcome of the flow: >> MOZ_ASSERT(lastIndex <= INT32_MAX); Since in both cases type of lastIndex is int32_t and INT32_MAX is: >> #define INT32_MAX 2147483647i32 MOZ_ASSERT becomes useless. Although code is used only on debug we should still remove it.

Review commit: https://reviewboard.mozilla.org/r/30673/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/30673/

Maybe it would make sense to change this assertion to test that lastIndex does not go above the Max string length?

(In reply to Nicolas B. Pierron [:nbp] from comment #2) > Maybe it would make sense to change this assertion to test that lastIndex > does not go above the Max string length? 1. For path js::RegExpTesterRaw test is done in function ExecuteRegExp(): >> /* Handled by caller */ >> MOZ_ASSERT(lastIndex >= 0 && size_t(lastIndex) <= input->length()); >> >> /* Steps 4-10 performed by the caller. */ 2. For path js::RegExpMatcherRaw test is done in function RegExpMatcherImpl()->ExecuteRegExp() So i guess we are safe to avoid any further checks.

Seems to me like lastIndex should be uint32_t. I'm fairly sure we pass int32_t as uint32_t in some JIT entry points, and this seems like another place where that should be done. That would also make the assertion reasonable.

Comment on attachment 8715768 [details] [diff] [review] as index cannot be negative make change it's storage from int32_t to uint32_t, Review of attachment 8715768 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/builtin/RegExp.cpp @@ +932,4 @@ > MatchPairs* maybeMatches, MutableHandleValue output) > +{ > + MOZ_ASSERT(lastIndex <= INT32_MAX); > + Kill the trailing whitespace here. (Not sure how any line but the |uint32_t lastIndex| line here got changed, to be honest.) ::: js/src/builtin/RegExp.h @@ +55,5 @@ > RegExpTester(JSContext* cx, unsigned argc, Value* vp); > > extern bool > RegExpTesterRaw(JSContext* cx, HandleObject regexp, HandleString input, > + uint32_t lastIndex, bool sticky, uint32_t* endIndex); |endIndex| here shouldn't be changed to uint32_t. RegExp testing returns -1 in the case where no match was found, so it makes sense for this to be signed. ::: js/src/jit/CodeGenerator.cpp @@ +1915,5 @@ > } > }; > > typedef bool (*RegExpTesterRawFn)(JSContext* cx, HandleObject regexp, HandleString input, > + uint32_t lastIndex, bool sticky, uint32_t* result); And then we should continue to have |int32_t* result| here. ::: js/src/jit/Recover.cpp @@ +1040,5 @@ > RootedString string(cx, iter.read().toString()); > RootedObject regexp(cx, &iter.read().toObject()); > int32_t lastIndex = iter.read().toInt32(); > bool sticky = iter.read().toBoolean(); > + uint32_t endIndex; And |int32_t| again.

Comment on attachment 8707389 [details] MozReview Request: Bug 1239269 - as lastIndex cannot be negative change it's storage class from int32_t to uint32_t, r?Waldo Review request updated; see interdiff: https://reviewboard.mozilla.org/r/30673/diff/1-2/

Comment on attachment 8707389 [details] MozReview Request: Bug 1239269 - as lastIndex cannot be negative change it's storage class from int32_t to uint32_t, r?Waldo https://reviewboard.mozilla.org/r/30673/#review31155